refactor: expose Kubeconform as a module #189

aabouzaid · 2023-04-10T04:03:36Z

Hi again 👋

As mentioned in PR no. #170, instead of dealing with KRM inside Kubeconform, it's better to expose it as a module and use it externally.

In the PR:

Expose the main logic of Kubeconform to be used as an app (not as lib) inside other applications (e.g. KubeconformValidator (a Kustomize plugin).
Expose Kubeconform config with proper YAML and JSON tags so it's easy to unmarshal
No breaking changes, all original behaviours are kept as it is.

Fixes: #160

Best Regards.

yannh · 2023-04-11T18:22:55Z

Hi @aabouzaid , Kubeconform can be used as a module, there is an example here: https://github.com/yannh/kubeconform/blob/master/examples/main.go - would it work for you? Your PR is pretty extensive and I'm not sure I agree with all changes...
Congrats on shipping though 👏 !!

aabouzaid · 2023-04-11T20:46:33Z

@yannh thanks for the link, but it's only the Kueconform individual packages that are exposed, not the app itself.
A big chunk of the code in main is not exposed.

That means everyone who wants to use Kueconform needs to implement the same execution logic that uses the underneath Kubeconform packages. This is a huge redundancy IMO (I'm not sure what is the actual reason not to expose the app but only its libs).

Also, for example, the config is exposed, but it doesn't have any YAML or JSON tags, which also means extra work if someone wants to extend Kubeconform.

So given that I don't try to use Kubeconform in my app but actually writing an interface to use Kubeconform, it's not even helpful to reimplement what you already did (KubeconformValidator is just a wrapper for Kubeconform).

All changes here are minimal to make Kubeconform app reusable, so if anything is not clear, I'd be happy to clarify the reasoning behind the changes (and even remove some of the changes if they don't fit for one reason or another).

Thank you 🙇

pkg/config/config.go

aabouzaid · 2023-04-12T21:51:37Z

@yannh I hope that's better 🙇
The PR doesn't change any existing behaviour.

yannh · 2023-04-22T14:56:27Z

Quite a lot to like about that PR thanks 🙇 Can you expand on this "NGConfig", why that is interesting? Have a few nits here and there, but I can see where you're going. Adding a few comments for discussion in the code.

yannh · 2023-04-22T14:58:14Z

pkg/config/config.go

+	Summary                bool     `yaml:"summary" json:"summary"`
+	Verbose                bool     `yaml:"verbose" json:"verbose"`
+	Version                bool     `yaml:"version" json:"version"`
+	Stream                 *Stream


I'm wondering if Stream fits better here, or as a separate argument to Validate().

Maybe in cmd/kubeconform if it is exposed (it should be a package, not main to be reusable).

I will think of a struct and it has something like:

type Kubeconform struct { Stream *Stream Config *Config }

which represents Kubeconform as an application.

yannh · 2023-04-22T14:59:00Z

pkg/output/output.go

-func New(outputFormat string, printSummary, isStdin, verbose bool) (Output, error) {
-	w := os.Stdout
-
+func New(w io.Writer, outputFormat string, printSummary, isStdin, verbose bool) (Output, error) {


I quite like the ability to override the streams here 👍

yannh · 2023-04-22T14:59:42Z

pkg/config/config.go

+}
+
+// LoadNGConfig allows to introduce new config format/structure while keeping backward compatibility.
+func (c *Config) LoadNGConfig() error {


I would suggest leaving this out for now, to make the PR easier to merge.

yannh · 2023-04-22T15:02:34Z

main.go

@@ -0,0 +1,24 @@
+package main


I kinda liked having the main function in cmd/kubeconform... I might move the validation to pkg/kubeconform.. still thinking about this.

I saw different ways to make it, but if that's your preference, maybe putting the main back in cmd/kubeconform and just moving validate to its own package under cmd/kubeconform/validate.

Another idea, we can just put Validate() under pkg/validator/validator.go.

I've not completely made up my mind on this, I'm likely to try out something that would allow you to work, but I might reserve myself the right to break that location/interface for a little while while I find a good place for it :)

aabouzaid · 2023-04-23T02:30:01Z

Quite a lot to like about that PR thanks bow Can you expand on this "NGConfig", why that is interesting? Have a few nits here and there, but I can see where you're going. Adding a few comments for discussion in the code.

The idea of the NG config is to overcome the diversion between the input of Kubeconform and the actual data structure in the Config struct.

Let's take Config.SkipKinds as an example.
The field SkipKinds represents -skip CLI argument. -skip takes a "comma-separated list of kinds or GVKs to ignore", which simply should be presented as a list of string []string, however, it's presented as map[string]struct{}.

I've read most of the code, and it's not clear to me why that data type is used if the skipped items don't have extra config.
the only thing I can think about is that it's just used to make it easy to check with the if condition since Golang doesn't provide a method to check if an item is in an array, but a smart way to make it is to use a map!

Here is an example of what I mean:

kubeconform/pkg/validator/validator.go

Line 118 in e3bb348

if _, ok := val.opts.SkipKinds[signature.GroupVersionKind()]; ok {

So what is the problem here?
the problem is that the data structure will look odd in YAML:

skip:
  ConfigMap:
  Service:
  Deployment:

which is confusing if anyone looked at the arg equivalent, which is -skip "ConfigMap,Service,Deployment".

The idea of NG (new generation) config is to bridge the new with the old format so skip could be presented in YAML as:

skip:
- ConfigMap
- Service
- Deployment

This will work out of the box, it will not introduce any breaking changes, and a deprecation note could be added.
Later after a couple of versions, the old method would be removed, and the new one would be used by default.

If anything is not clear, feel free to mention it, I'd be happy to add more details,

yannh · 2023-04-23T11:29:28Z

I'll start committing parts of these to main in small commits, I ll put you as co-committer 🙇 Not impossible this will trigger a few conflicts though, but it is easier for me to do in small chunks.

yannh · 2023-04-23T11:46:20Z

I do get the feeling behind the array. The reason it was a map was for easy lookups in here.

kubeconform/pkg/validator/validator.go

Line 117 in 8bc9f42

skip := func(signature resource.Signature) bool {

. There should be no need for backward compatibility at the moment.

yannh · 2023-04-23T12:11:32Z

main.go

+	}
+
+	if err = kubeconform.Validate(cfg, out); err != nil {
+		fmt.Fprintf(os.Stderr, "failed validating resources: %s - %s\n", err.Error(), out)


this is incorrect.. kubeconform currently returns 1 when some resources are not valid (but it didn't fail validating resources) - we need to differentiate errors processing from errors when validating..
It could return a bool saying if everything was valid or not?

I didn't change the logic here, I just used what's there but with an error instead of an init.
So any return 1 became a return error and any return 0 became a return nil.

yannh · 2023-04-23T12:40:23Z

cmd/kubeconform/validate.go

@@ -93,13 +83,11 @@ func realMain() int {
 		useStdin = true


we still refer to os.Stdin here instead of cfg.stream.stdin, but here we need Stat()

yeah, I put it away to think about the best way to make it.
I will get back to it.

Hold on a bit - I'm not super happy with the signature of "Validate" yet, I would want it to enable the caller to process the results in a way they see fit. I need to think about this a bit more 🙇

great 👌 👌

aabouzaid mentioned this pull request Apr 10, 2023

Feat: Support KRM input #170

Closed

aabouzaid force-pushed the aa-expose-validator branch 2 times, most recently from 6280f79 to 7029e22 Compare April 11, 2023 16:21

aabouzaid commented Apr 12, 2023

View reviewed changes

pkg/config/config.go Outdated Show resolved Hide resolved

refactor: expose kubeconform as a module

484252d

aabouzaid force-pushed the aa-expose-validator branch from 7029e22 to 66e2a96 Compare April 12, 2023 21:44

refactor: expose kubeconform config

336abef

aabouzaid force-pushed the aa-expose-validator branch from 66e2a96 to 336abef Compare April 13, 2023 12:32

yannh reviewed Apr 22, 2023

View reviewed changes

yannh reviewed Apr 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: expose Kubeconform as a module #189

refactor: expose Kubeconform as a module #189

aabouzaid commented Apr 10, 2023 •

edited

Loading

yannh commented Apr 11, 2023

aabouzaid commented Apr 11, 2023 •

edited

Loading

aabouzaid commented Apr 12, 2023

yannh commented Apr 22, 2023

yannh Apr 22, 2023

aabouzaid Apr 23, 2023

yannh Apr 22, 2023

yannh Apr 22, 2023

yannh Apr 22, 2023

aabouzaid Apr 23, 2023

aabouzaid Apr 23, 2023

yannh Apr 23, 2023

aabouzaid commented Apr 23, 2023 •

edited

Loading

yannh commented Apr 23, 2023

yannh commented Apr 23, 2023

yannh Apr 23, 2023 •

edited

Loading

aabouzaid Apr 23, 2023

yannh Apr 23, 2023

aabouzaid Apr 24, 2023

yannh Apr 24, 2023

aabouzaid Apr 24, 2023

refactor: expose Kubeconform as a module #189

Are you sure you want to change the base?

refactor: expose Kubeconform as a module #189

Conversation

aabouzaid commented Apr 10, 2023 • edited Loading

yannh commented Apr 11, 2023

aabouzaid commented Apr 11, 2023 • edited Loading

aabouzaid commented Apr 12, 2023

yannh commented Apr 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aabouzaid commented Apr 23, 2023 • edited Loading

yannh commented Apr 23, 2023

yannh commented Apr 23, 2023

yannh Apr 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aabouzaid commented Apr 10, 2023 •

edited

Loading

aabouzaid commented Apr 11, 2023 •

edited

Loading

aabouzaid commented Apr 23, 2023 •

edited

Loading

yannh Apr 23, 2023 •

edited

Loading